Skip to content

Completion for errors#2619

Merged
Techatrix merged 1 commit intozigtools:masterfrom
psznm:error-complete
Feb 26, 2026
Merged

Completion for errors#2619
Techatrix merged 1 commit intozigtools:masterfrom
psznm:error-complete

Conversation

@psznm
Copy link
Contributor

@psznm psznm commented Feb 21, 2026 •
edited
Loading

This adds completion for errors.

There are issues demonstrated by tests

Issue test 11

"switch on error set - 11" is catch |err| switch (err) construct.

I suspect that scope detection might be wrong which is why "switch on error set - 11" does not work. innermostScopeAtIndex in lookupSymbolGlobal returns scope that looks garbage when printed out.

This:

    var current_scope = innermostScopeAtIndex(document_scope, source_index);
    const node = document_scope.getScopeAstNode(current_scope);
    if (node) |nod| {
        const src = handle.tree.getNodeSource(nod);
        std.debug.print("\nSRC=---\n{s}\n---\n", .{src});
    }

Prints out

       SRC=---
       {
         idk

Trying to tackle the issue is intimidating to me as I don't even know where I would start. Maybe someone more experienced could give some leads how to solve the issue?

Issue test 13

"switch on error set - 13" is completion in a function in a structure.

This also feels like issue with scopes. I started digging into how scopes are created. I found DocumentScope where it walks the document. It seems like structure declarations are completely ignored, seemingly on the std side, because the walkContainerDecl just walks the const std = @import("std") and then just stops. When I print out container_decl.ast.members.len , it gives 1. When I delete the std import, there is 0. Nothing gets walked.

After some more digging, it seems both issues are basically the same one. The 13 is structure member that is not given to zls from std. The 11 seems like a block statement that is not given to zls from std (the walker sees test_decl and its block, but nothing else). Is there any chance of fixing this from zls side, or does this need to be fixed on std side?

EDIT: Okay, I found out that the Ast parser is not error-prone enough for zls. Both issues are caused by that the parser refuses to give us these specific members/blocks that contain invalid syntax. So this is to be improved on the stdlib side. But here we can at least add completions for "empty" context to auto-complete errors. Those should work with both of the problematic cases, and honestly, even be the most ergonomic completions. I'll see if I can do that.

@psznm psznm force-pushed the error-complete branch 2 times, most recently from 651fa59 to 5a09b52 Compare February 21, 2026 13:37
@psznm psznm changed the title Complete errors Basic completion for errors Feb 21, 2026
@psznm psznm force-pushed the error-complete branch 3 times, most recently from 6cb5139 to b453d80 Compare February 21, 2026 14:19
@psznm psznm changed the title Basic completion for errors Completion for errors Feb 21, 2026
@psznm psznm force-pushed the error-complete branch 3 times, most recently from 685a0a3 to 0ed0f5f Compare February 21, 2026 14:55
Techatrix
Techatrix reviewed Feb 24, 2026
View reviewed changes
Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of enumerating in test names, each test name should have a descriptive name based on what it is trying to test. You may also consider group tests together if they are similar enough.

Here is the summary of the suggested code changes (plus some minor refactors):

for (containers) |container| {
    const ip = builder.analyser.ip;
    const key = switch (container.data) {
        .ip_index => |info| ip.indexToKey(info.type),
        else => return,
    };
    if (key != .error_set_type) return;
    const names = try key.error_set_type.names.dupe(builder.arena, ip);
    try builder.completions.ensureUnusedCapacity(builder.arena, names.len);
    for (names) |interned_name| {
        const name = try std.fmt.allocPrint(builder.arena, "{f}", .{interned_name.fmt(ip.io, &ip.string_pool)});
        if (used_members_set.contains(name)) continue;
        builder.completions.appendAssumeCapacity(.{
            .label = try std.fmt.allocPrint(builder.arena, "error.{s}", .{name}),
            .kind = .Constant,
            .insertText = try std.fmt.allocPrint(builder.arena, "{s} => ", .{name}),
        });
    }
}

This is small enough that the collectErrorSetNames function can just be inlined manually.

@psznm
Copy link
Contributor Author

psznm commented Feb 25, 2026

Instead of enumerating in test names, each test name should have a descriptive name based on what it is trying to test. You may also consider group tests together if they are similar enough.

Here is the summary of the suggested code changes (plus some minor refactors):

for (containers) |container| {
    const ip = builder.analyser.ip;
    const key = switch (container.data) {
        .ip_index => |info| ip.indexToKey(info.type),
        else => return,
    };
    if (key != .error_set_type) return;
    const names = try key.error_set_type.names.dupe(builder.arena, ip);
    try builder.completions.ensureUnusedCapacity(builder.arena, names.len);
    for (names) |interned_name| {
        const name = try std.fmt.allocPrint(builder.arena, "{f}", .{interned_name.fmt(ip.io, &ip.string_pool)});
        if (used_members_set.contains(name)) continue;
        builder.completions.appendAssumeCapacity(.{
            .label = try std.fmt.allocPrint(builder.arena, "error.{s}", .{name}),
            .kind = .Constant,
            .insertText = try std.fmt.allocPrint(builder.arena, "{s} => ", .{name}),
        });
    }
}

This is small enough that the collectErrorSetNames function can just be inlined manually.

Thanks, I had the code similiar enough after solving other comments. I still took heavy inspiration from it, but not exactly as you wrote it. I also kept it as a function, because I felt the scopes were kinda awkward with continues at different levels when inlined. So i feel function is much nicer here.

@psznm psznm requested a review from Techatrix February 25, 2026 04:09
@psznm psznm mentioned this pull request Feb 25, 2026
Open
@Techatrix Techatrix merged commit 0d1367e into zigtools:master Feb 26, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Techatrix Techatrix Techatrix approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@psznm @Techatrix